Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize the api proto descriptors in swarmkit #2349

Merged
merged 3 commits into from
Aug 15, 2017

Conversation

tossmilestone
Copy link
Contributor

@tossmilestone tossmilestone commented Aug 11, 2017

Use the API stability tool of protobuild to stabilize the proto descriptors in swarmkit. It will generate:

  • api/api.pb.txt for api package

Signed-off-by: He Xiaoxi [email protected]

@tossmilestone
Copy link
Contributor Author

@stevvooe PTAL, thanks!

@tossmilestone tossmilestone force-pushed the stabilize-api branch 8 times, most recently from 775003a to 086831b Compare August 11, 2017 16:27
@codecov
Copy link

codecov bot commented Aug 11, 2017

Codecov Report

Merging #2349 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #2349     +/-   ##
=========================================
- Coverage    60.1%   60.01%   -0.1%     
=========================================
  Files         128      128             
  Lines       26177    26177             
=========================================
- Hits        15734    15709     -25     
- Misses       9045     9070     +25     
  Partials     1398     1398

Protobuild.toml Outdated

[[descriptors]]
prefix = "github.com/docker/swarmkit/protobuf/plugin"
target = "protobuf/plugin/plugin.pb.txt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to stabilize the plugin API, they are only used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok,I will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe updated, please review again

@stevvooe
Copy link
Contributor

LGTM

Copy link
Contributor

@nishanttotla nishanttotla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tossmilestone @stevvooe do we need to include the api/api.pb.txt file in the repo?

@stevvooe
Copy link
Contributor

@nishanttotla That is the entire point of the PR. ;) The idea is that symbol changes in api/api.pb.txt will identify possible breaking API changes.

Please see the README in containerd. That explains it more thoroughly. Perhaps, this PR should include a similar readme.

@tossmilestone
Copy link
Contributor Author

tossmilestone commented Aug 15, 2017

@stevvooe I will add a similar README in containerd later to clarify how to use it.

Copy link
Contributor

@nishanttotla nishanttotla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @tossmilestone for adding the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants